Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: (anti)feature - remove poetry.lock from solution template #506

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nick-marnik
Copy link
Contributor

I think I was the guilty person who originally added the poetry.lock file to the solution template. I stand by this decision, but I think that over time circumstances have changed a bit and I have noted a few negatives to this approach:

  • adding/updating packages to pyproject.toml is annoying because you need to instantiate a copy of the template and update the poetry.lock file manually
  • packages defined in pyproject.toml with version range specifiers do not ever get updated

This PR removes the poetry.lock file from the template. However, due to the way the setup_environment.py script works, a new poetry.lock file will be generated upon first use of the template. This approach fixes the above negatives:

  • maintainers can simply edit packages in pyproject.toml without any other changes
  • packages will be upgraded to latest available versions based on the defined constraints. These upgrades will occur at the time of instantiation of the template - after that, it is up to the solution maintainers (or preferably dependabot) to keep the dependencies in the poetry.lock file updated

There is one main risk/drawback to the new approach: the template itself is no longer deterministic (stable). The solution template is now dependent on when it is instantiated. I don't think this is inherently bad, but is something we will need to keep in mind. Specifically, we will need to write the version ranges for our packages very carefully to ensure that the package version upgrades we consume automatically are compatible with the template contents.

If we ever find a package upgrade that breaks the solution template, I don't think the remedy is to add the poetry.lock file back. Instead, we should update the version range specification for said package to prevent the issue.

I also think that the E2E tests created by the framework team are now even more important than before. If there are package incompatibilities, they should be surfaced by the E2E solution tests. We should confirm that those tests are all functional, running on a cron schedule, and the results are being monitored by someone on the framework team.

@nick-marnik nick-marnik requested a review from a team as a code owner September 23, 2024 21:16
@nick-marnik
Copy link
Contributor Author

nick-marnik commented Sep 23, 2024

I forgot to include them, but here is a before/after of the resulting package versions (from pip freeze) before/after deleting the poetry.lock file:
packages-new.txt
packages-old.txt

The diff shows the extent of how outdated the current lock file is:
image

@nick-marnik
Copy link
Contributor Author

@vgelbgras The E2E tests are failing. I dug into the error a bit, but can't find any issues. I can reproduce the error using the validation repo - the only discrepancy I noticed is that the poetry.lock file generated by the E2E tests lists 1.5.2 in the header, but we should be using 1.8.3

@nick-marnik
Copy link
Contributor Author

Or it may be a true test incompatibility with the upgrade from glow-engine 1.11.2 to 1.12.0 - over 250+ files changed, so it is tough for me to see if there is a breaking change lurking in the changelog somewhere: https://github.com/ansys-internal/glow-engine/compare/v1.11.2...v1.12.0

@vgelbgras
Copy link
Collaborator

@vgelbgras The E2E tests are failing. I dug into the error a bit, but can't find any issues. I can reproduce the error using the validation repo - the only discrepancy I noticed is that the poetry.lock file generated by the E2E tests lists 1.5.2 in the header, but we should be using 1.8.3

@nick-marnik
The tests should be adjusted to avoid using private module.

https://github.com/ansys-internal/basic-solution-template-validator/blob/3dff0609e99a033d7c3259a90c2c6fc423cc0633/tests/conftest.py#L16
image

@nick-marnik
Copy link
Contributor Author

@vgelbgras I am not familiar with those tests - instead of removing them, is there someone involved in writing them who could over some insight into how they should be refactored?

@vgelbgras
Copy link
Collaborator

Hi @nick-marnik
Maybe @ccpansys could provide some guidances

@ccpansys
Copy link
Contributor

Hi @nick-marnik, the entry point for this tests is the solution-templates-end-to-end-testing repository. I have not been tasked with maintaining this repository since SAF teams reorganized, but feel free to ask me if there is anything you need help with. I have given write access to the repository to the solutions developers team.

The only thing that would need to be directly modified on the basic-solution-template-validator are the workflow files which are committed to the main branch and are retrieved from solution-templates-end-to-end-testing while creating the solution to be pushed (This looks like a clunky way to work, I do not recall if there was a constraint in the way actions work that pushed me to implement it like that. May have been a rookie thing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants